Add Autofill support #3241
Add Autofill support #3241
Conversation
|
I think this needs a rebase pls. I'll wait to review until we have new chromium and electron builds avail. |
|
I don't think this is actually ready to merge yet. I'm still working with @darkdh on a few issues on the electron side and I'm pretty sure he is still working on the browser side as well. |
|
@bridiver I just solved the locking issue in electron by brave/muon@d1991b7. And I think browser side can be reviewed for its function aspect while I'm working on making it looks better :) |
| email: string, | ||
| guid: Object.<string, <string>> // map of (partition, id) used to access the autofill entry in database | ||
| }], | ||
| creditCards: [{ |
There was a problem hiding this comment.
we can't store plain text CC data in the state. It's already encrypted on the electron side so we should either store the guid and read from there or encrypt (base64 encoded) here. Preferably the former I think because we don't really need two copies.
There was a problem hiding this comment.
just FYI - this is a blocker for merge because it's a major security issue
There was a problem hiding this comment.
Fixed in 90f62be. Thanks.
|
Can we add some tests for this? A few things that should be covered:
|
| items.push({ | ||
| label: value, | ||
| click: (item, focusedWindow) => { | ||
| ipc.send('autofill-selection-clicked', frame.get('tabId'), value, frontendId, i) |
There was a problem hiding this comment.
can we get rid of the ipc message and do a window action here that calls webContents.autofillSelect in frames.js? Maybe track the autofill status in the frame state and show/hide the context menu based on that? Could definitely be done in a follow-up commit, but we will have to refactor the ipc eventually anyway when moving webContents mgmt to the browser process
|
@bridiver please review again but it seems like the credit card thing is resolved so I'm going to merge so we can get 0.11.6 testing in on it for the beta3 build. |
git rebase -ito squash commits if needed.fix #860
auditors: @bridiver, @bbondy